-
Notifications
You must be signed in to change notification settings - Fork 845
refactor: share EVM rpc package #4786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
alarso16
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to register the types for our tests? I think that was the only thing preventing us from using the geth rpc package
The only place we registered was in an additional main file just in subnet-evm, and that was for test retrying for flakes. I removed that as we should not be doing test retries in the monorepo, and should just skip flaky tests entirely if they're flaky. Is there registration anywhere else? |
No, we didn't just retry failed tests. They also registered |
I don't think this is true. There are two places in this PR where I removed
Are there other important places in which I removed it? |
|
If This can alternately be done with func testCChainAndSubnetEVM(t *testing.T, testFn func(*testing.T)) {
fn := func() error {
testFn(t)
return nil
}
t.Run("cchain", func(t *testing.T) {
_ = emulate.CChain(fn)
})
t.Run("subnetEVM", func(t *testing.T) {
_ = emulate.SubnetEVM(fn)
})
}
func TestFoo(t *testing.T) {
testCorethAndSubnetEVM(t, testFoo)
}
func testFoo(t *testing.T) {
// this is where the actual testing occurs; i.e. just rename any existing TestFoo to testFoo
}
func TestBar(t *testing.T) {
testCorethAndSubnetEVM(t, testBar)
}
func testBar(t *testing.T) {
// as with testFoo()
}The linter is going to tell you to call |
|
I believe we have decided to enforce that we don't want cyclic dependencies between |
Yeah I am trying this right now and was about to comment about the cyclic imports. The shared I definitely think this + in conjunction with some sort of test main would be a good model for how to combine packages while testing separately. I've committed a new |
Signed-off-by: Jonathan Oppenheimer <[email protected]>
| notifier.Notify(id, msg) | ||
| have := strings.TrimSpace(out.String()) | ||
| want := `{"jsonrpc":"2.0","method":"_subscription","params":{"subscription":"test","result":{"parentHash":"0x0000000000000000000000000000000000000000000000000000000000000001","sha3Uncles":"0x0000000000000000000000000000000000000000000000000000000000000000","miner":"0x0000000000000000000000000000000000000000","stateRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","transactionsRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","receiptsRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000","difficulty":null,"number":"0x64","gasLimit":"0x0","gasUsed":"0x0","timestamp":"0x0","extraData":"0x","mixHash":"0x0000000000000000000000000000000000000000000000000000000000000000","nonce":"0x0000000000000000","extDataHash":"0x0000000000000000000000000000000000000000000000000000000000000000","baseFeePerGas":null,"extDataGasUsed":null,"blockGasCost":null,"blobGasUsed":null,"excessBlobGas":null,"parentBeaconBlockRoot":null,"timestampMilliseconds":null,"minDelayExcess":null,"hash":"0x9a9ca1b5790a674785245afedcee9fc1a90d3514c6faa1cbd26f696136d6fd12"}}}` | ||
| want := `{"jsonrpc":"2.0","method":"_subscription","params":{"subscription":"test","result":{"parentHash":"0x0000000000000000000000000000000000000000000000000000000000000001","sha3Uncles":"0x0000000000000000000000000000000000000000000000000000000000000000","miner":"0x0000000000000000000000000000000000000000","stateRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","transactionsRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","receiptsRoot":"0x0000000000000000000000000000000000000000000000000000000000000000","logsBloom":"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000","difficulty":null,"number":"0x64","gasLimit":"0x0","gasUsed":"0x0","timestamp":"0x0","extraData":"0x","mixHash":"0x0000000000000000000000000000000000000000000000000000000000000000","nonce":"0x0000000000000000","baseFeePerGas":null,"withdrawalsRoot":null,"blobGasUsed":null,"excessBlobGas":null,"parentBeaconBlockRoot":null,"hash":"0xe5fb877dde471b45b9742bb4bb4b3d74a761e2fb7cb849a3d2b687eed90fb604"}}}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ceyonur suggested bring back the original TestNotify, rather than deleting it entirely, when testing without registration.
| @@ -9,7 +9,7 @@ | |||
| // | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub is drunk showing this move...
|
After extensive discussion, we have decided not to test under registration. |
graft/subnet-evm/accounts/abi/bind/precompilebind/precompile_bind_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Austin Larson <[email protected]> Signed-off-by: Jonathan Oppenheimer <[email protected]>
alarso16
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The go.mod changes are somewhat suspicious, but I don't think an actual issue
ceyonur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Why this should be merged
The subnet-evm
rpcpackage was practically identical to the corethrpcpackage. It makes no sense to maintain two copies of this code. This also makes moving to libevm'srpcpackage far easier, to look in one location and see what needs to be libevmified.How this works
graft/evm/rpc/directory structurecoreth/rpcto the shared locationgithub.com/gorilla/websocket v1.5.0github.com/deckarep/golang-set/v2 v2.1.0golang.org/x/time v0.12.0TestNotifyto original libevm versiontest_main.gofor registration during testing.Outdated information
I also added `main_test.go` with a `TestMain` that automatically runs all RPC tests twice - once with C-Chain type registration and once with Subnet-EVM type registration.This works by putting
TestMaininpackage rpc_test(an external test package) instead ofpackage rpc, which lets us import theemulatepackage without creating a circular dependency. We can then useemulate.CChain()andemulate.SubnetEVM()to properly register/cleanup types between test runs.Thus when you run
go test(which also automatically happens via CI -- you can check for yourself!), it:emulate.CChain()emulate.SubnetEVM()How this was tested
CI
Need to be documented in RELEASES.md?
No